-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Migrate UnsizedConstParamTy
to unstable impl of ConstParamTy_
#145095
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
Support using #[unstable_feature_bound] on trait This is needed to unblock rust-lang#145095 r? `@BoxyUwU`
Support using #[unstable_feature_bound] on trait This is needed to unblock rust-lang#145095 r? ``@BoxyUwU``
Support using #[unstable_feature_bound] on trait This is needed to unblock rust-lang#145095 r? ```@BoxyUwU```
Support using #[unstable_feature_bound] on trait This is needed to unblock rust-lang#145095 r? ````@BoxyUwU````
Support using #[unstable_feature_bound] on trait This is needed to unblock rust-lang#145095 r? `````@BoxyUwU`````
Support using #[unstable_feature_bound] on trait This is needed to unblock rust-lang#145095 r? ``````@BoxyUwU``````
Support using #[unstable_feature_bound] on trait This is needed to unblock rust-lang#145095 r? ```````@BoxyUwU```````
Support using #[unstable_feature_bound] on trait This is needed to unblock rust-lang/rust#145095 r? ```````@BoxyUwU```````
Support using #[unstable_feature_bound] on trait This is needed to unblock rust-lang/rust#145095 r? ```````@BoxyUwU```````
a4bd0a8
to
262cf11
Compare
Ok(()) | ||
}) | ||
} else if tcx.features().adt_const_params() { | ||
if tcx.features().adt_const_params() || tcx.features().unsized_const_params() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should make unsized_const_params
no longer be a lang feature. you can do this by removing its entry from unstable.rs
Currently all tests in |
☔ The latest upstream changes (presumably #145773) made this pull request unmergeable. Please resolve the merge conflicts. |
if tcx.features().adt_const_params() | ||
|| tcx.features().enabled(sym::unsized_const_params) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that we need to have this change in order for the test here not to request for adt_const_params
to be enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that test should just enable adt_const_params
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird, why do we no longer emit error for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Foo
implements ConstParamTy with no unstable_feature_bound so is useable by all downstream crates. This is fine I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a diagnostic regression here, it'd be nice to keep the help message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diagnostic is a bit chatty here, we already have the same diagnostic above, it'd be nice to only say it once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this happen because we have both #[unstable]
and #[unstable_feature_bound]
on the trait, and they emit two errors separately. I think I will just leave this as it is and write a test later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is in https://github.com/rust-lang/rust/pull/146401/files, I think we can leave this as it is, and improve the diagnostic in another PR.
@@ -101,7 +102,8 @@ pub fn type_allowed_to_implement_const_param_ty<'tcx>( | |||
lang_item: LangItem, | |||
parent_cause: ObligationCause<'tcx>, | |||
) -> Result<(), ConstParamTyImplementationError<'tcx>> { | |||
assert_matches!(lang_item, LangItem::ConstParamTy | LangItem::UnsizedConstParamTy); | |||
assert_matches!(lang_item, LangItem::ConstParamTy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to accept a lang_item argument if it can only ever be one thing. just replace all uses of lang_item with LangItem::ConstParamTy
library/core/src/tuple.rs
Outdated
@@ -45,17 +45,11 @@ macro_rules! tuple_impls { | |||
maybe_tuple_doc! { | |||
$($T)+ @ | |||
#[unstable(feature = "adt_const_params", issue = "95174")] | |||
#[unstable_feature_bound(adt_const_params)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[unstable_feature_bound(adt_const_params)] | |
#[unstable_feature_bound(unsized_const_params)] |
dont want this impl insta-stable when we stabilize adt const params. its not really "unsized" but right now its the only other feature gate we can put impls under 😅
I expect we'll wind up with a bunch of different feature gates for "support X
if tcx.features().adt_const_params() | ||
|| tcx.features().enabled(sym::unsized_const_params) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that test should just enable adt_const_params
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Foo
implements ConstParamTy with no unstable_feature_bound so is useable by all downstream crates. This is fine I think
@@ -142,7 +139,7 @@ fn visit_implementation_of_const_param_ty( | |||
checker: &Checker<'_>, | |||
kind: LangItem, | |||
) -> Result<(), ErrorGuaranteed> { | |||
assert_matches!(kind, LangItem::ConstParamTy | LangItem::UnsizedConstParamTy); | |||
assert_matches!(kind, LangItem::ConstParamTy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
u can remove the kind
parameter, no need to accept an argument that can only be one value
@@ -5,14 +5,6 @@ LL | struct Foo<const NAME: &'static str>; | |||
| ^^^^^^^^^^^^ | |||
| | |||
= note: the only supported types are integers, `bool`, and `char` | |||
help: add `#![feature(adt_const_params)]` to the crate attributes to enable more complex and user defined types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
driveby: can you change this test to use a const generic parameter that would actually be supported under adt_const_params
. e.g. struct Foo<const N: Bar>; struct Bar(u8);
parent_cause.clone(), | ||
param_env, | ||
ty::ClauseKind::UnstableFeature(sym::unsized_const_params), | ||
)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the diagnostic regression where we lost the help: enable this feature
is caused by the fact that type_allowed_to_implement_const_param_ty
no longer returns a special error for when its not allowed because of a unsized_const_params only type.
calling ocx.select_where_possible
here and returning ConstParamTyImplementationError::UnsizedConstParamsFeatureRequired
if there were any errors should fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah. needs to be ocx.select_all_or_error
actually. i forgot that unstable feature bounds never return NoSolution
. You only register the bound here, you dont actually try and prove it and then return a custom failure reason which is what will bring back all the "help" messages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amazing, this works!
@@ -1,27 +1,19 @@ | |||
warning: the feature `unsized_const_params` is incomplete and may not be safe to use and/or cause compiler crashes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah hmm :(
I kinda forgot that switching to a library feature would mean we no longer consider feature(unsized_const_params) to be incomplete 🤔 Do you think you could re-add unsized_const_params
as a langauge feature just so that we continue getting these warnings? It shouldn't actually be used anywhere in the compiler, I just want people to get pushed away from actually using this for now lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I brought it back now. Initially I was thinking if there is any way to have similar error message for libs feature, but I didn't find anything. 🤔
error[E0741]: `Bar` must implement `ConstParamTy` to be used as the type of a const generic parameter | ||
--> $DIR/feature-gate-unsized-const-params.rs:5:21 | ||
| | ||
LL | struct Foo<const N: [u8]>; | ||
| ^^^^ | ||
LL | struct Foo<const N: Bar>; | ||
| ^^^ | ||
| | ||
= note: the only supported types are integers, `bool`, and `char` | ||
help: add `#![feature(adt_const_params)]` to the crate attributes to enable more complex and user defined types | ||
help: add `#[derive(ConstParamTy, PartialEq, Eq)]` to the struct | ||
| | ||
LL + #![feature(adt_const_params)] | ||
| | ||
help: add `#![feature(unsized_const_params)]` to the crate attributes to enable references to implement the `ConstParamTy` trait | ||
| | ||
LL + #![feature(unsized_const_params)] | ||
LL - struct Bar(u8); | ||
LL + #[derive(ConstParamTy, PartialEq, Eq)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The help message here is a bit weird, but feels like a pre-existing issue, can take a look at this after tihs PR.
This reverts commit 675ad69.
8d88452
to
1f70531
Compare
Now that we have
#[unstable_feature_bound]
, we can removeUnsizedConstParamTy
that was meant to be an unstable impl of stable type andConstParamTy_
trait.r? @BoxyUwU